Conversation
MimallocArena.init() used mi_heap_new() which creates a heap with tag 0 — the same tag as the backing heap. When worker threads exit, mimalloc routes their abandoned pages to the first heap matching the tag via _mi_heap_by_tag. With tag 0, those pages get routed to a MimallocArena instead of the backing heap. When mi_heap_destroy is later called on that arena, it frees the reclaimed pages and their live blocks, corrupting the malloc free-list. Use mi_heap_new_ex with a non-zero tag (111) so abandoned pages from dead threads are always routed to the backing heap (tag 0) where they belong. Closes #28153
|
Updated 7:13 AM PT - Mar 16th, 2026
❌ @autofix-ci[bot], your commit e6c1b36 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28154That installs a local version of the PR into your bun-28154 --bun |
|
✅ All local gates pass on Gate summary:
|
WalkthroughReplaces plain heap creation with a tagged mimalloc heap (tag 111) in the allocator and adds a regression test that exercises heap behavior during worker teardown and concurrent transpilation. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28153.test.ts`:
- Around line 58-65: The spawn call for proc via Bun.spawn does not pipe stdout,
so proc.stdout will be null when later awaiting proc.stdout.text(); update the
Bun.spawn options used where proc is created to include stdout: "pipe"
(alongside stderr: "pipe") so proc.stdout is available, ensuring the Promise.all
that awaits proc.stdout.text(), proc.stderr.text(), and proc.exited succeeds.
- Around line 67-68: Update the assertions to include stderr in failure
diagnostics: when asserting stdout.trim() === "PASS" and exitCode === 0
(references to stdout.trim() and exitCode), make the assertion messages include
stderr so failures print useful debugging output (or explicitly
log/console.error stderr before/when the expect for exitCode fails); ensure you
reference the existing stderr variable so any non-zero exitCode or unexpected
stdout shows the stderr contents in the test output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fec708c1-062d-4861-a7e2-851d47eabfa7
📒 Files selected for processing (2)
src/allocators/MimallocArena.zigtest/regression/issue/28153.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The one-line fix looks correct — using mi_heap_new_ex(111, true, null) to assign a unique heap tag prevents abandoned pages from dead worker threads being misrouted to MimallocArena heaps — but since this touches low-level allocator internals where a mistake means heap corruption, a human should verify the root cause analysis and that tag 111 has no conflicts.
Extended reasoning...
Overview
This PR changes a single function call in src/allocators/MimallocArena.zig: mi_heap_new() is replaced with mi_heap_new_ex(arena_heap_tag, true, null) where arena_heap_tag = 111. A regression test is added in test/regression/issue/28153.test.ts. The total logic change is one line.
Security risks
The change is in memory allocator code. If the fix is wrong (e.g., tag 111 conflicts with another heap tag, or allow_destroy has unexpected side effects with the new API), it could introduce new heap corruption rather than fixing it. The API signature matches the extern declaration in mimalloc.zig:226 and the parameters look correct, but the stakes are high.
Level of scrutiny
This deserves careful human review despite its small size. Allocator internals are among the most safety-critical code in a runtime — a single wrong parameter could cause use-after-free, double-free, or heap corruption that only manifests under specific timing conditions. The PR description provides a detailed and plausible root cause analysis, but it should be verified by someone familiar with mimalloc's heap tagging and page reclamation internals.
Other factors
- The PR has the
claudelabel, indicating it was likely auto-generated, which increases the importance of human verification. - The regression test is acknowledged as best-effort (the real bug requires ~10 hours of sustained load to trigger), so the test alone doesn't provide strong confidence.
- No other callers of
mi_heap_new()exist in the codebase outside the mimalloc wrapper itself, so this is the only site that needed the fix. - The magic number 111 for the tag is arbitrary but should be confirmed not to collide with any other heap tags used in the codebase or by mimalloc internally.
There was a problem hiding this comment.
LGTM — single-line fix with correct mi_heap_new_ex signature, unique tag prevents abandoned page misrouting.
Extended reasoning...
Overview
This PR changes one line in src/allocators/MimallocArena.zig: replacing mi_heap_new() (which creates heaps with tag 0) with mi_heap_new_ex(111, true, null) (non-zero tag). A regression test is added at test/regression/issue/28153.test.ts.
Security risks
None. The change improves memory safety by preventing heap corruption from mimalloc page misrouting. The fix is purely defensive — it prevents abandoned pages from dead worker threads (tag 0) from being incorrectly reclaimed by MimallocArena heaps.
Level of scrutiny
This is a small, targeted allocator fix. The mi_heap_new_ex extern signature (heap_tag: c_int, allow_destroy: bool, arena_id: mi_arena_id_t) was verified against the Zig bindings. Tag 111 is unique — no other code in the repository uses mi_heap_new_ex. The allow_destroy=true preserves the existing mi_heap_destroy behavior in deinit(). CI passes across all platforms.
Other factors
All CodeRabbit review comments were false positives (Bun.spawn defaults stdout to pipe, not inherit) and were resolved. The regression test follows established patterns (tempDir, bunExe/bunEnv, await using, checking stdout before exit code). The root cause analysis in the PR description is thorough and matches the fix.
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
3 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.